Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement the InvokeHostFunctionOp part of CAP-52 #3450

Merged
merged 16 commits into from
Jun 17, 2022
Merged

Implement the InvokeHostFunctionOp part of CAP-52 #3450

merged 16 commits into from
Jun 17, 2022

Conversation

jonjove
Copy link
Contributor

@jonjove jonjove commented Jun 14, 2022

Description

Implements the InvokeHostFunctionOp part of CAP-52, but does not include the host functions.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@jonjove jonjove requested a review from graydon June 14, 2022 22:48
@jonjove jonjove mentioned this pull request Jun 14, 2022
Merged
graydon
graydon requested changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good! A few minor touchups.

src/protocol-next/xdr/Stellar-transaction.x Show resolved Hide resolved
src/transactions/InvokeHostFunctionOpFrame.cpp Outdated Show resolved Hide resolved
src/transactions/test/InvokeHostFunctionTests.cpp Show resolved Hide resolved
src/transactions/InvokeHostFunctionOpFrame.cpp Outdated Show resolved Hide resolved
jayz22
jayz22 reviewed Jun 15, 2022
View reviewed changes
src/rust/src/contract.rs Outdated
/// Deserializes an [`xdr::HostFunction`] host function identifier, an [`xdr::ScVec`] XDR object of
/// arguments, an [`xdr::Footprint`] and a sequence of [`xdr::LedgerEntry`] entries containing all
/// the data the invocation intends to read. Then calls the host function with the specified
/// arguments and serializes the [`xdr::ScVal`] return value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is inaccurate, the return value of the host function call is thrown away. Instead the ReadWrite set is serialized and returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

@graydon
Copy link
Contributor

graydon commented Jun 16, 2022

LGTM -- I leave the landing sequence to you, I think you might need to merge the other 2 first and then bump the Cargo.toml here to point to the post-merge revision ID of the env crate.

@sisuresh
Copy link
Contributor

r+ a68b53e

@sisuresh
Copy link
Contributor

r+ b1d22f7

@latobarita latobarita merged commit 7684e14 into stellar:master Jun 17, 2022
@jonjove jonjove mentioned this pull request Sep 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayz22 jayz22 jayz22 left review comments

@sisuresh sisuresh sisuresh left review comments

@leighmcculloch leighmcculloch leighmcculloch left review comments

@graydon graydon graydon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jonjove @graydon @sisuresh @leighmcculloch @latobarita @jayz22